-
Notifications
You must be signed in to change notification settings - Fork 3.4k
feat: allow Cmd+L / Cmd+I on empty selection to select the entire line #6711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
I think some of the e2e tests have a valid failure here. But, I believe at least for Is your main thought here that Copilot behaves in the way you showed in that video? cc @sestinj for a second opinion on behavior. |
Chatted this over with @sestinj
With that in mind, could we scope the update to basically be "if the user pressed |
So from what I can see, the existing Cmd+L behavior is to show the webview if hidden, and focus the cursor there. Not sure it's accurate to call it toggle, since another Cmd+L doesn't hide it, but the webview is unfocused. I can look into reducing the scope of Cmd+I, I'll see if it works |
Hm, this isn't working for you? For me if I repeatedly hit |
I merged main but I believe the remaining failures are legitimate. |
So actually the bulk of these changes concerns selecting the whole line on Cmd+L. For Cmd+I, the only thing that changed was the command is now visible when right-clicking on the editor (removing the editorHasSelection clause for continue.focusEdit in package.json). The behavior of Cmd+I itself has not changed. If the new Cmd+L behavior is not something the Continue team is willing to get in, this is fine, we can just reject this PR. But if you find some value in this approach, then I'll look more closely into the test failures, as expectations have changed. @Patrick-Erichsen Let me know what you decide. |
No it doesn't work for me that way. 2nd Cmd+L doesn't close the sidebar, just unfocusses it. I tried woth both release and prerelease versions of continue, on 2 macbooks. |
Hm, well the But if the |
ok, I'll take a look at the test failures when I'm back from PTO on Monday |
bf886bd
to
96c3627
Compare
Think a few are still failing unfortunately |
Yes, it was a simple rebase. I need to figure out how to run those tests locally |
Signed-off-by: Fred Bricon <[email protected]>
Signed-off-by: Fred Bricon <[email protected]>
96c3627
to
1696eb2
Compare
@fbricon after sitting on this for a while we're thinking it should add the whole file, since in most cases during usage the user will want to just add the file to context, and one line is rarely useful/probably usually confusing to the model. Thoughts on tweaking to add full file instead of that line? Agree that removing the sidebar toggle functionality is fine |
This PR makes it simpler to add code to the Chat context: if no code is selected, the entire line is added to the context by Cmd+I or Cmd+L. This behaviour is similar to what Github Copilot offers.
expand-empty-selection.mp4
Summary by cubic
Cmd+L and Cmd+I now select the entire line if no text is selected, making it easier to add code to the Chat context. This matches the behavior found in GitHub Copilot.